Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Signed integers #1718

Merged
merged 33 commits into from
Jun 30, 2023
Merged

Signed integers #1718

merged 33 commits into from
Jun 30, 2023

Conversation

chipshort
Copy link
Collaborator

@chipshort chipshort commented Jun 9, 2023 •
edited
Loading

closes #1710

  • reimplemented uints using bnum crate
  • started adding ints (also based on bnum)
  • had to update CI rust version to 1.65 for it to compile
  • commented out tarpaulin in CI for now, because the version we use does not use rust 1.65, but we might not be able to update easily (see Upgrade tarpaulin to 0.22.0+ #1455)

webmaster128
webmaster128 reviewed Jun 13, 2023
View reviewed changes
Copy link
Member

@webmaster128 webmaster128 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔥

packages/std/src/errors/std_error.rs Outdated Show resolved Hide resolved
packages/std/src/math/int512.rs Show resolved Hide resolved
packages/std/src/math/int512.rs Outdated Show resolved Hide resolved
packages/std/src/math/int512.rs Show resolved Hide resolved
packages/std/src/math/int512.rs Outdated Show resolved Hide resolved
packages/std/src/math/int512.rs Outdated Show resolved Hide resolved
packages/std/src/math/int512.rs Show resolved Hide resolved
packages/std/src/math/int512.rs Show resolved Hide resolved
packages/std/src/math/uint256.rs Show resolved Hide resolved
chipshort and others added 23 commits June 28, 2023 17:45
@chipshort
Port big uint types to bnum
78c9adf
@chipshort
Implement Int512
6b9b234
@chipshort
Update ci rust version
1f157e7
@chipshort
Disable coverage check temporarily
560d1d9
@chipshort
Simplify uint impls
ee059cd
@chipshort
Cleanup int512
77be691
@chipshort
Update rust in github workflow
776d629
@chipshort
Improve int512 tests
92ef1c6
@chipshort
Add Shl and Neg impl for Int512
59f7f83
@chipshort
Int512::checked_div_euclid
bcbb3e1
@chipshort
Add checked_neg to Int512
98e877f
@chipshort
Remove uint dependency
630e299
@chipshort
Use forward_ref macros for Int512 impls
d530dbc
@chipshort
Improve DivisionError
5bcb9d2
@chipshort
Improve error string
6b38adf
@chipshort @webmaster128
Inline Int512 constructor
67fe1b4 
Co-authored-by: Simon Warta <2603011+webmaster128@users.noreply.github.com>
@chipshort
Add non_exhaustive to OverflowOperation
c65232f
@chipshort
Simplify tests
7a379c5
@chipshort
Add Int256
ac2b0be
@chipshort
Fix typo
49867d9
@chipshort
Add Int128
b395d8e
@chipshort
Add Int64
a5ae44a
@chipshort
Add trait impl check for signed ints
8d89684
@chipshort chipshort force-pushed the chipshort/signed-math branch from 2c7c97c to 8d89684 Compare June 28, 2023 16:14
@chipshort chipshort changed the base branch from main to release/1.3 June 28, 2023 16:14
@chipshort chipshort marked this pull request as ready for review June 28, 2023 16:14
@chipshort
Copy link
Collaborator Author

I added the missing signed ints and rebased on 1.3

webmaster128
webmaster128 reviewed Jun 29, 2023
View reviewed changes
packages/std/Cargo.toml Outdated Show resolved Hide resolved
packages/std/src/math/int128.rs Outdated Show resolved Hide resolved
packages/std/src/math/int128.rs Outdated Show resolved Hide resolved
packages/std/src/math/int256.rs Outdated Show resolved Hide resolved
packages/std/src/math/int256.rs Outdated Show resolved Hide resolved
packages/std/src/math/int64.rs Outdated Show resolved Hide resolved
packages/std/src/math/mod.rs Show resolved Hide resolved
webmaster128
webmaster128 reviewed Jun 29, 2023
View reviewed changes
Copy link
Member

@webmaster128 webmaster128 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very very nice stuff 🙌 Let's add a CHANGELOG entry and merge

maurolacy
maurolacy reviewed Jun 30, 2023
View reviewed changes
Copy link
Contributor

@maurolacy maurolacy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partial review up to int64 (without tests) inclusive.

Looks good. Will review the other int types later. And take a look at the tests.

packages/std/src/math/uint512.rs Show resolved Hide resolved
packages/std/src/math/int64.rs Show resolved Hide resolved
packages/std/src/math/int64.rs Show resolved Hide resolved
packages/std/src/math/int64.rs
/// This operation will panic if `rhs` is zero.
#[inline]
fn rem(self, rhs: Self) -> Self {
Self(self.0.rem(rhs.0))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Rely on checked_rem for consistency?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to keep it consistent with the unsigned ones (they have the same implementation for rem).

Actually, I think it might be better to change the ones using self.0.checked_* to use either self.0.*, a self.checked_* call or an explicit message using .expect. That would give more helpful panic messages.
But I think we can do that later.

packages/std/src/math/int64.rs
}
forward_ref_op_assign!(impl ShlAssign, shl_assign for Int64, u32);

impl Serialize for Int64 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps not worth it, but, move all the serialisations to a int_se module?

packages/std/src/math/int64.rs
}
}

impl<'de> Deserialize<'de> for Int64 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, own int_de module? Not sure about this, just saying.

packages/std/src/math/int64.rs
Self: Add<A, Output = Self>,
{
fn sum<I: Iterator<Item = A>>(iter: I) -> Self {
iter.fold(Self::zero(), Add::add)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice.

hashedone
hashedone approved these changes Jun 30, 2023
View reviewed changes
Copy link
Contributor

@hashedone hashedone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I hope I didn't miss any typos in indexing sizes, but I don't see any. I am wondering if we could just reexport bnum types. However, I see we do error alignment in checked functions, so maybe it is not the best idea.

@chipshort chipshort merged commit cad5513 into release/1.3 Jun 30, 2023
@chipshort chipshort deleted the chipshort/signed-math branch June 30, 2023 13:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@webmaster128 webmaster128 webmaster128 left review comments

@maurolacy maurolacy maurolacy approved these changes

@hashedone hashedone hashedone approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Signed integers: Int64, Int128, Int256, Int512
4 participants
@chipshort @webmaster128 @hashedone @maurolacy